[alpha.webkit.UncheckedCallArgsChecker] Don't emit a warning for passing a temporary object as an argument. #155033
Conversation
…ing a temporary object as an argument. Since a temporary object lives until the end of the statement, it's safe to pass such an object as a function argument without explicitly creating a CheckedRef/CheckedPtr in stack.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesSince a temporary object lives until the end of the statement, it's safe to pass such an object as a function argument without explicitly creating a CheckedRef/CheckedPtr in stack. Full diff: https://github.com/llvm/llvm-project/pull/155033.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 478bd85177143..0d2294e1e9bb0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -208,6 +208,8 @@ bool isASafeCallArg(const Expr *E) {
return true;
}
}
+ if (isa<CXXTemporaryObjectExpr>(E))
+ return true; // A temporary lives until the end of this statement.
if (isConstOwnerPtrMemberExpr(E))
return true;
diff --git a/clang/test/Analysis/Checkers/WebKit/unchecked-call-arg.cpp b/clang/test/Analysis/Checkers/WebKit/unchecked-call-arg.cpp
index 8685978ebf1ac..b89a3246606ed 100644
--- a/clang/test/Analysis/Checkers/WebKit/unchecked-call-arg.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/unchecked-call-arg.cpp
@@ -32,3 +32,8 @@ void foo() {
provide()->doWork();
// expected-warning@-1{{Call argument for 'this' parameter is unchecked and unsafe}}
}
+
+void doWorkWithObject(const CheckedObject&);
+void bar() {
+ doWorkWithObject(CheckedObject());
+}
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/39247 Here is the relevant piece of the build log for the reference |
| @@ -208,6 +208,8 @@ bool isASafeCallArg(const Expr *E) { | |||
| return true; | |||
| } | |||
| } | |||
| if (isa<CXXTemporaryObjectExpr>(E)) | |||
| return true; // A temporary lives until the end of this statement. | |||
There was a problem hiding this comment.
Actually no, it could be lifetime extended. Moreover, since C++17 due to mandatory copy elision it can end up absolutely anywhere. Stack, heap, global, an unknown location into which the function 12 levels above you in the call stack returns, you name it. For example: https://godbolt.org/z/EnvdsWK8G - notice how there are no copy/move-constructors anywhere in that AST. The CXXTemporaryObjectExpr (a sub-class of CXXConstructExpr btw) constructs the object directly at its final destination, no matter how far apart they are in the AST.
The annoying part is that you can't tell that by looking at the construct-expression itself. Normally for a method call the this argument is inside the call expression but for constructors it's somewhere in the parent expression and it could be one of like 30+ different rigid AST variations that you need to take into account, often with an indeterminate amount of irrelevant parent statement layers you need to peel one by one. (Eg. there could be an arbitrary amount of nested ?: operators.) And I'm afraid you can't really tell how long the object lives until you pattern-match the specific AST pattern to figure out the construction target.
Frontend solves it by simply being recursive - so it always has the construction target somewhere up its visitor stack. Maybe that approach would work for you too? You could use the parent map too but that could lead you to some really weird places. For our less recursive analysis machines I've tried to learn how to pattern-match some of these cases in https://clang.llvm.org/doxygen/classclang_1_1ConstructionContext.html but I wasn't wise enough to make it available over pure AST. And it's a very incomplete list anyway.
So the remaining question is, is there a specific way to quickly notice the temporaries that aren't lifetime-extended and ignore the other cases? Basically what you're looking for is the nearest parent MaterializeTemporaryExpr - and then you can ask it whether its getStorageDuration() is SD_FullExpression. The annoying part is that MaterializeTemporaryExpr doesn't have to be there at all - consider C(14, 15) and C(16, 17) in the godbolt example. In this case if you keep using the parent map it may lead you to weird places. So I think your best bet may be to either find an somewhat immediate MaterializeTemporaryExpr or give up and treat the constructor as unsafe. That'd probably handle your test case but it'd fail if doWorkWithObject() accepted it by value. For that you'd need to handle the "passed directly into a CallExpr" case too. But if you don't find either of those almost immediately (either directly above you, or additionally bypassing CXXBindTemporaryExpr which shows up iff the class has a non-trivial destructor IIRC) then it might be better to give up and treat the constructor is unsafe.
There was a problem hiding this comment.
Actually no, it could be lifetime extended. Moreover, since C++17 due to mandatory copy elision it can end up absolutely anywhere. Stack, heap, global, an unknown location into which the function 12 levels above you in the call stack returns, you name it.
Oh sure, but here what we need is the temporary to at least out-live the end of current statement since what we want to guarantee is that each function argument outlives the function call. It's okay if the temporary's lifetime is extended beyond that.
There was a problem hiding this comment.
Perhaps we can revise the comment to say it "lives at least until the end of this statement"?
There was a problem hiding this comment.
Oooooooooh. Right. You already know it's an argument. Yeah, case closed, the codegen-style recursive approach wins again.
And it looks like you've correctly skipped the MaterializeTemporaryExpr which is probably there in your test because the argument is accepted by reference. There may also be a CXXBindTemporaryExpr if the class has a destructor but it's probably peeled correctly for the same reason.
You folks are really good at this 😅
…ing a temporary object as an argument. (llvm#155033) Since a temporary object lives until the end of the statement, it's safe to pass such an object as a function argument without explicitly creating a CheckedRef/CheckedPtr in stack.
Since a temporary object lives until the end of the statement, it's safe to pass such an object as a function argument without explicitly creating a CheckedRef/CheckedPtr in stack.